Skip to content

Use make install#17

Closed
gvol wants to merge 6 commits intogap-system:masterfrom
gvol:8afa4f7
Closed

Use make install#17
gvol wants to merge 6 commits intogap-system:masterfrom
gvol:8afa4f7

Conversation

@gvol
Copy link
Copy Markdown

@gvol gvol commented Apr 22, 2023

This switches from building in place to using make install to install to the Cellar. This should fix #14. Please let me know if I installed the packages to the wrong location or something.

I took the approach of listing every package explicitly, and whether it used the new or old style configure. It seemed like that was consensus on how it should work, but I'm happy to change it to work more like BuildPackages.sh.

It's built on top of #16 because I didn't want to have to rebase everything. If that's not desired, then I can get rid of the --HEAD option, I don't have too strong of feelings.

I'm a little worried that I didn't explicitly require all the dependencies I should have, but I just didn't notice because I already had them installed on my machine.

Two packages just don't work yet, namely xgap and cddinterface.

I have lightly tested the installation and it seems sane, though some of the singular packages fail their tests due to some minor differences (perhaps my singular is older or new than the one used when writing the tests).

Closes #16
Resolves #15
Resolves #14
Resolves #1

Ivan Andrus and others added 2 commits April 21, 2023 22:53
This is a first step towards getting the formula approved for use in
homebrew proper
Comment thread Formula/gap.rb Outdated
Comment thread Formula/gap.rb Outdated
Comment thread Formula/gap.rb Outdated
@dimpase
Copy link
Copy Markdown
Member

dimpase commented Dec 16, 2023

I've tried this PR, and while it builds, I get

homebrew-gap % gap                      
 ┌───────┐   GAP 4.12.2 of 2022-12-18
 │  GAP  │   https://www.gap-system.org
 └───────┘   Architecture: x86_64-apple-darwin23-default64-kv8
 Configuration:  gmp 6.3.0, GASMAN, readline
 Loading the library and packages ...
 Packages:   AClib 1.3.2, Alnuth 3.2.1, AutPGrp 1.11, CRISP 1.4.6, Cryst 4.1.25, CrystCat 1.1.10, FactInt 1.6.3, 
             FGA 1.4.0, GAPDoc 1.6.6, IRREDSOL 1.4.4, LAGUNA 3.9.5, Polenta 1.3.10, Polycyclic 2.16, 
             PrimGrp 3.4.3, RadiRoot 2.9, ResClasses 4.7.3, SmallGrp 1.5.1, Sophus 1.27, TransGrp 3.6.3, 
             utils 0.81
 Try '??help' for help. See also '?copyright', '?cite' and '?authors'
gap> LoadPackage("packagemanager");
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 1st choice method found for `Filename' on 2 arguments
The 1st argument is 'fail' which might point to an earlier problem
 at /usr/local/Cellar/gap/4.12.2/libexec/gap/share/gap/lib/methsel2.g:249 called from
<function "HANDLE_METHOD_NOT_FOUND">( <arguments> )
 called from read-eval loop at /usr/local/Cellar/gap/4.12.2/libexec/gap/lib/gap/pkg/packagemanager/gap/PackageManager.gd:295
type 'quit;' to quit to outer loop
brk> 

Some other packages, e.g. orb and grape, appear to work.

Also, shouldn't liggap.* be installed/linked in /usr/local/lib/ rather than only in /usr/local/Cellar/gap/4.12.2/libexec/gap/lib

@fingolfin
Copy link
Copy Markdown
Member

This PR seems like a great improvement, so it would be a shame to let this go to waste.

@fingolfin
Copy link
Copy Markdown
Member

@gvol if you are still interested in this, I could just give you write access, so you don't have to wait for me to even notice you have done work or submitted a PR.

Also @pedritomelenas has offered to help out, I'd be happy to give him as well

Comment thread Formula/gap.rb


no_compilation_packages = [
"atlasrep", "aclib", "agt", "alnuth", "automata", "automgrp",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such a long list seems error prone and annoying because it needs to be updated for each GAP release.

Can't you instead just compute a list of the names of all subdirs of the pkg directory on the fly? Resp. not even use that list, just iterate over them all

Comment thread Formula/gap.rb

# The makefiles appear to only be used for docs...
# The BuildPackages.sh script didn't call them
all_packages.each do |pkg|
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So here one could instead iterate over all subdirs (I am sure there is a ruby way to do that)

Comment thread Formula/gap.rb
if File.exist?("./prerequisites.sh")
system "./prerequisites.sh", "#{libexec}/gap/lib/gap"
end
end
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of having the configure_packages and configure_packages lists, here you could have a similar check to see if there is a ./configure file. If there is, you can distinguish "old" and "new" style by checking of configure contains the string with-gaproot...

Though actually, with GAP 4.13.0, all but one configure script support --with-gaproot -- the one exception is that of simpcomp, which however happily ignores it. So you could just ditch the whole old_configure_packages part.

@fingolfin
Copy link
Copy Markdown
Member

I am leaving this in here a little bit more, to give @gvol time to react, if he likes to. But otherwise we can perhaps merge this next week, and then perhaps 4.13.1 is out and someone can update this here.

The suggestions I just made of course don't have to be addressed in this PR right away, though I hope they are actually not that hard to realize.

@pedritomelenas
Copy link
Copy Markdown
Collaborator

Thanks!

@fingolfin
Copy link
Copy Markdown
Member

@pedritomelenas I gave you access

@gvol I'll give you access if you let me know

Anyone else interested in contributing: reach out to me and we'll figure something out

I guess an immediate job would be to merge this PR; then perhaps perform the changes I suggest above (or to record those change suggestions in an issue or two). And also to update this to GAP 4.13.1 which is about to be released in a few hours at most.

@fingolfin
Copy link
Copy Markdown
Member

@pedritomelenas @gvol ping

@dimpase
Copy link
Copy Markdown
Member

dimpase commented Jan 29, 2025

$Ping^2$ ? Or should I take over from here?

@pedritomelenas
Copy link
Copy Markdown
Collaborator

Please, go ahead, as I already confessed on slack, this is out of my reach

@gvol
Copy link
Copy Markdown
Author

gvol commented Apr 23, 2025

You are welcome to take it. I keep thinking I'll get back to it eventually, but it obviously hasn't happened.

@olexandr-konovalov
Copy link
Copy Markdown
Member

The PR's description says it closes #16 and resolves three issues (which are now resolved). Do we still need #16? @gvol @fingolfin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ensure more packages are built correctly Rewrite to use make install Add new dependencies to build more packages

5 participants